-
Notifications
You must be signed in to change notification settings - Fork 40
feat: add first draft of drop order in or patterns clarification #631
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: add first draft of drop order in or patterns clarification #631
Conversation
|
Hi Pete! Here is some feedback, which attempts to make the new text a bit more formal. Note that I am actually introducing more rules to cover the missing semantics for or-patterns. Changes to I would put the definition of "or-pattern" immediately after that of "pattern", with the following new text: After (note that the Reference states "function and closure arguments", but I think they mean function and closure parameters) Change Insert the following text after Add new section Changes to Add the following text before Change The example you provided should be cleaned up a bit, and should be an explanation similar to the existing example. |
|
Thank you for the extensive feedback @kirtchev-adacore 🥰 I'll review and then update or discuss as needed. |
|
one thing we should decide is if we should add (new) code examples to FLS, and if so, in what cases |
|
I think that these new drop semantics are sufficiently complex that they deserves an example. In fact, I had to run the example code in the Rust Playground to ensure I understood the semantics correctly. |
375cc20 to
59e9df0
Compare
Thanks again @kirtchev-adacore -- I've made a revision at: 59e9df0
Haven't done this one yet. Maybe we can discuss in today's meeting. |
src/glossary.rst
Outdated
| ^^^^^^^^^^ | ||
|
|
||
| :dp:`fls_LnPDQW3bnNUw` | ||
| An :dt:`or pattern` is a :t:`pattern` that matches on one of two or more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
via @kirtchev-adacore -- add the following here:
is a :t:`pattern` which or-s two or more :t:`[subpattern]s` using
character 0x7C (vertical line).
And then mirror the glossary into the text where this definition is used.
(part of why @tshepang wants the glossary as a source document to go away, but render it instead from the body of the text)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did this in: c7f7717
src/patterns.rst
Outdated
|
|
||
| :dp:`fls_72JHo343O7jp` | ||
| An or-pattern shall not appear in the :t:`pattern-without-alternation` of a | ||
| :t:`closure parameter`, a :t:`function parameter`, or a :t:`let statement`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requires definition of let binding.
| :t:`closure parameter`, a :t:`function parameter`, or a :t:`let statement`. | |
| :t:`closure parameter`, a :t:`function parameter`, or a :t:`let binding`. |
Thanks to @traviscross, @ehuss for pointing out the difference and how handled in the Reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You will have to define what a "let binding" is. 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried my hand at the definition in: 900088b
Happy to hear feedback
| } | ||
| let c = PrintOnDrop("2"); | ||
| :dp:`fls_THzA0QFdMMJB` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to rework into step-by-step explanation as in the example under Drop Order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Attempted this here: aba5ea5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me. 👍
What do others think?
src/glossary.rst
Outdated
| ^^^^^^^^^^^ | ||
|
|
||
| :dp:`fls_sw6HrsxsnG2y` | ||
| A :dt:`let binding` is a :t:`construct` that introduces :t:`[binding]s` by matching a :t:`value` against a :t:`pattern` using the keyword ``let``. :t:`[let binding]s` appear in :t:`[let statement]s`, :t:`[if let expression]s`, :t:`[while let loop expression]s`, and :t:`[let initializer]s`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of things here:
I do not think that a let binding is a construct. The construct (the "syntactic thing") is actually PatternWithoutAlternation. "let binding" is a semantic notion for certain PatternWithoutAlternations (those that appear in let statements, if let expressions, etc).
I do not think that let initializer should be included in the context list since the syntax of LetInitializer does not contain a PatternWithoutAlternation.
How about something along the lines of:
A let binding is the binding introduced by a let statement, an if let expression, or a while let loop expression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks more faithful. Adopted.
| } | ||
| let c = PrintOnDrop("2"); | ||
| :dp:`fls_THzA0QFdMMJB` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me. 👍
What do others think?
|
@ehuss @traviscross Should the term for "or patterns" have a hyphen? Basically "or-pattern" instead of "or pattern". |
|
We use a hyphen for "or-patterns". (aside: TC and I spent entirely too much time yesterday discussing if it is |
|
Agreed. Added this entry to our WIP style guide: or-patternsThe hyphen is appropriate in the compound term or-patterns, even when it acts as a noun, to avoid a miscue. The word or is acting as a noun adjunct but the reader is going to want to see it as a coordinating conjunction. And-gate and or-gate, as nouns, are also hyphenated for this reason. Note that this is a special circumstance. A term such as while loop is not hyphenated except when acting as a phrasal adjective preceding a noun, as in "the while-loop expression is...". |
|
Thanks for the clarification! @PLeVasseur , I think there are a few uses of "or pattern" in the PR. |
Co-authored-by: Hristian Kirtchev <60669983+kirtchev-adacore@users.noreply.github.com>
Co-authored-by: Hristian Kirtchev <60669983+kirtchev-adacore@users.noreply.github.com>
Co-authored-by: Hristian Kirtchev <60669983+kirtchev-adacore@users.noreply.github.com>
Co-authored-by: Hristian Kirtchev <60669983+kirtchev-adacore@users.noreply.github.com>
Co-authored-by: Hristian Kirtchev <60669983+kirtchev-adacore@users.noreply.github.com>
Co-authored-by: Hristian Kirtchev <60669983+kirtchev-adacore@users.noreply.github.com>
Co-authored-by: Hristian Kirtchev <60669983+kirtchev-adacore@users.noreply.github.com>
kirtchev-adacore
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from two small nits, the PR looks great to me! Well done Pete! Trial by fire 😆
| let c = PrintOnDrop("2"); | ||
| :dp:`fls_THzA0QFdMMJB` | ||
| When an :t:`or pattern` is used, the drop order of :t:`[binding]s` is determined by the first :t:`pattern-without-alternation`, regardless of which matches at runtime. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| When an :t:`or pattern` is used, the drop order of :t:`[binding]s` is determined by the first :t:`pattern-without-alternation`, regardless of which matches at runtime. | |
| When an :t:`or-pattern` is used, the drop order of :t:`[binding]s` is determined by the first :t:`pattern-without-alternation`, regardless of which matches at runtime. |
Apparently I missed this during my precious reviews 😢
| When the first call matches ``Ok([a, b])``, the :t:`[binding]s` are dropped in reverse declaration order: ``b`` then ``a``. | ||
|
|
||
| #. :dp:`fls_gNWXh61ZXXt8` | ||
| When the second call matches ``Err([b, a])``, the drop order remains ``b`` then ``a``, determined by the first :t:`pattern-without-alternation`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| When the second call matches ``Err([b, a])``, the drop order remains ``b`` then ``a``, determined by the first :t:`pattern-without-alternation`. | |
| When the second call matches ``Err([b, a])``, the drop order remains ``b`` then ``a`` since it is determined by the first :t:`pattern-without-alternation`. |
I think this is slightly clearer.
| or-pattern | ||
| ^^^^^^^^^^ | ||
|
|
||
| :dp:`fls_LnPDQW3bnNUw` | ||
| An :dt:`or-pattern` is a :t:`pattern` that matches on one of two or more :t:`[pattern-without-alternation]s` and or-s them using character 0x7C (vertical line). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it's worth including the literal | here somewhere.
| All other :t:`bindings` are :t:`dropped` in reverse declaration order. | ||
|
|
||
| * :dp:`fls_W2S2FrkuedYC` | ||
| :t:`[Binding]s` introduced by an :t:`or-pattern` are dropped in reverse | ||
| declaration order, where the declaration order is defined by the first | ||
| :t:`pattern-without-alternation`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might seem odd to refer to "all other bindings" ahead of speaking to what bindings aren't the other ones. It might be worth reordering this.
| let c = PrintOnDrop("2"); | ||
| :dp:`fls_THzA0QFdMMJB` | ||
| When an :t:`or pattern` is used, the drop order of :t:`[binding]s` is determined by the first :t:`pattern-without-alternation`, regardless of which matches at runtime. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels maybe a bit repetitive with what's above.
| #. :dp:`fls_tZJgZDWVChJV` | ||
| If the :t:`pattern` is an :t:`or-pattern`, then perform :t:`or-pattern matching`. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This duplicates the algorithm that falls immediately below it. The text that follows is:
For each
pattern-without-alternationof the pattern:
That is, it's meaning to loop over all of the subpatterns of an or-pattern.
|
|
||
| .. _fls_VsBXBj4AqCj1: | ||
|
|
||
| Or-pattern Matching | ||
| ~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| :dp:`fls_njpiXkgi8Ryb` | ||
| :dt:`Or-pattern matching` of an :t:`or-pattern` of the form `constructor(or-pattern, rest)`, | ||
| where `constructor` is an arbitrary constructor, `or-pattern` is the :t:`or-pattern`, | ||
| and `rest` is optionally a remaining pattern, proceeds as follows: | ||
|
|
||
| #. :dp:`fls_nE7qZpHy9TPu` | ||
| Perform pattern matching of the form `constructor(subpattern, rest)`, where | ||
| `subpattern` is a :t:`subpattern` of the :t:`or-pattern`, starting from the | ||
| first such :t:`subpattern` and proceeding in declarative order. | ||
|
|
||
| #. :dp:`fls_P8yB2b5enpw7` | ||
| Otherwise pattern matching fails. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this look outside of the or-pattern by describing it in terms of constructor(or-pattern, rest) (rather than just starting at the or-pattern)? At first, I thought maybe this was trying to follow some existing pattern in the chapter, but I don't see other instances of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had suggested that text to Pete, which I extracted from https://github.com/rust-lang/reference/blob/master/src/patterns.md#dynamic-semantics.
Stolen from in-progress rust-lang/fls#631
What this does
or patternwhich is intended to match the Reference's same conceptor patternin order to clarify the ordering of evaluation of thepattern-without-alternationor patterndrop order worksStill open, still thinking about it
I don't think I was able to get the definition of the
or patternto match 1-to-1 currently, as in the Referenceit refers to:
I didn't seen these concepts defined in that same way. I think it's at least possible there are similar concepts
in the FLS I haven't found yet.
For example, I think potentially
argument operandcould be used for referring to "function arguments".Will need to give this a bit more thought, but open to hear from others.
closes #623
Corresponding Reference PR: